refactor(sink): impl RowEncoder for JsonEncoder #12264
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12264 +/- ##
==========================================
+ Coverage 69.71% 69.73% +0.01%
==========================================
Files 1411 1413 +2
Lines 236351 236390 +39
==========================================
+ Hits 164783 164835 +52
+ Misses 71568 71555 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| } | ||
| } | ||
|
|
||
| fn datum_to_json_object( |
There was a problem hiding this comment.
All lines below (i.e. datum_to_json_object and mod tests) are moved from sink::utils without changing a single character.
|
|
||
| fn encode( | ||
| &self, | ||
| row: RowRef<'_>, |
There was a problem hiding this comment.
impl Row seems not enough here. It needs to be either &impl Row or impl Row + Copy. Each style appears exactly twice in existing code base. Do we have a preference here?
Some thoughts:
impl Row + Copycan be specified in caller, but&impl Rowmust be hereimpl Row + Copyaccepts non-ref but&impl Rowdoes notimpl Row + Copyis a superset of&impl Rowthanks toimpl<R: Row> Row for &R
src/connector/src/sink/utils.rs
Outdated
| @@ -405,26 +258,24 @@ pub async fn gen_upsert_message_stream<'a>( | |||
| pk_indices: &'a [usize], | |||
| chunk: StreamChunk, | |||
| _opts: UpsertAdapterOpts, | |||
| key_encoder: JsonEncoder, | |||
There was a problem hiding this comment.
May pass impl RowEncoder instead of JsonEncoder to make the code more general.
There was a problem hiding this comment.
This is included in later part of the refactor.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Part A of #12125 targeting #11995:
RowEncoder,SerTo<String>,SerTo<Vec<u8>>.JsonEncoderas an implementation ofRowEncoderpk_to_jsonandrecord_to_jsoninsink::utilsChecklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.